Skip to content

Conversation

@chenyukang
Copy link
Member

@chenyukang chenyukang commented Nov 7, 2025

Fixes #148634

The ICE happened because

attr::ReprSimd => ReprFlags::IS_SIMD,

will always set IS_SIMD according to get_all_attrs, and since we already report error attribute should be applied to a struct, it's OK to bypass here.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 7, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2025

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@Kivooeo
Copy link
Member

Kivooeo commented Nov 7, 2025

r=me when ci is green

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Nov 7, 2025

hmm, would it be possible to instead filter the ReprOptions so that we don't end up with unsupported ones? It feels brittle to simply handle incorrect info by ignoring it, e.g. it adds an implicit assumption that repr(simd) is only ever used for structs, and if we were to idk, extend support to unions at some point (even if unlikely) this code would suddenly be incorrect

alternatively, accept all Adts, but then match on the kind and skip + span_delay_bug if it's not a struct

@Kivooeo
Copy link
Member

Kivooeo commented Nov 7, 2025

it adds an implicit assumption that repr(simd) is only ever used for structs

according to the rfc it seems (at least for me) that this is only for structs https://github.com/rust-lang/rfcs/blob/master/text/1199-simd-infrastructure.md#types

@lcnr
Copy link
Contributor

lcnr commented Nov 7, 2025

Yes, but is this a guarantee that we'll never extend it to unions? My point is not that the code is wrong, it's that this code adds an implicit dependency which may result in bugs in the future.

Most bugs in large projects happen by:

  • somebody writes code which has an unchecked assumption which holds right now
  • somebody else modifies another part of the project, causing this assumption to no longer hold, resulting in a bug in entirely untouched code

This implies that a good way to avoid bugs in large projects it to avoid unchecked assumptions. It's why exhaustive matches are so powerful. It's also why we do stuff like

// This causes a compiler error if any new float kinds are added.
let (ty::FloatTy::F16 | ty::FloatTy::F32 | ty::FloatTy::F64 | ty::FloatTy::F128);
let possible_floats = [
ty::SimplifiedType::Float(ty::FloatTy::F16),
ty::SimplifiedType::Float(ty::FloatTy::F32),
ty::SimplifiedType::Float(ty::FloatTy::F64),
ty::SimplifiedType::Float(ty::FloatTy::F128),
];

@Kivooeo Kivooeo added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2025
@chenyukang chenyukang force-pushed the yukang-fix-148634-repr-simd-enum-ice branch from 995c118 to 112d833 Compare November 8, 2025 01:41
@chenyukang
Copy link
Member Author

@lcnr
I updated the code in commit:
https://github.com/rust-lang/rust/compare/995c11894fdabe1c630694254de756f82389c6cf..112d833ea5c72f89078069ffc5d10da5b1adf2ca

I'm thinking do we need to delay_as_bug, since if we already returned Err(NonAsmTypeReason::Invalid(ty)), there will always an error reported error[E0084]: unsupported representation for ...

@chenyukang
Copy link
Member Author

Ok, span_delay_bug seems checking the whether there is already an error before this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ICE: assertion failed: self.is_struct() || self.is_union()

5 participants